CSHARP-4432: POC of standard implementations of Equals. #1273
Conversation
rstam
requested review from
sanych-sun,
damieng,
adelinowona,
JamesKovacs and
BorisDog
and removed request for
a team
|
I'm about to implement a bunch of new Before doing so I'd like us to agree as a team on how we want to implement |
| !object.ReferenceEquals(other, null) && | ||
| this.GetType() == other.GetType() && | ||
| object.Equals(_ref1, other._ref1) && | ||
| _value1.Equals(other._value1); |
There was a problem hiding this comment.
Notes:
Some very old articles claim the following is faster than using object.ReferenceEquals:
(object)this == other
but most newer articles say that after JITing they are exactly the same thing. Using object.ReferenceEquals is more intention-revealing than the more obscure cast.
We must check that the types are equal. Objects of different types are never equal.
My recommendation is that we NEVER use == in the implementation fo Equals. Instead, we should always call Equals when comparing fields.
The reason is that in some cases == and Equals do NOT return the same value. When writing and reading Equals implementations it is better to standardize on calling Equals to compare fields instead of having to analyze field by field whether == semantics are correct.
Always use object.Equals(_ref1, other._ref1) instead of _ref1.Equals(other._ref1) when comparing reference values. That way nulls are handled correctly.
Always use _value1.Equals(other._value1) when comparing struct values. That way boxing is usually avoided (most struct classes will implement IEquatable<T>).
| object.ReferenceEquals(this, other) || | ||
| base.Equals(other) && | ||
| object.Equals(_ref2, other._ref2) && | ||
| _value2.Equals(other._value2); |
There was a problem hiding this comment.
Notes:
The object.ReferenceEquals(this, other) check must be coded at every level of a polymorphic class hierarchy for the optimization to apply at each level.
No need to check for null or compare types, that happens in base.Equals.
| object.ReferenceEquals(this, other) || | ||
| !object.ReferenceEquals(other, null) && | ||
| object.Equals(_ref, other._ref) && | ||
| _value.Equals(other._value); |
There was a problem hiding this comment.
No need to compare types because the class is non polymorphic.
This assumption is only valid is the class is sealed.
|
|
||
| public bool Equals(EquatableStruct other) => | ||
| object.Equals(_ref, other._ref) && | ||
| _value.Equals(other._value); |
There was a problem hiding this comment.
No need to check for null because this is a struct.
No need to compare types because structs can't be polymorphic.
|
I've re-requested a review from everyone on this PR. |
There was a problem hiding this comment.
Looks good. I did something similar a while back but had base classes implement CompareTo and get all the other operators https://github.com/damieng/DamienGKit/blob/master/CSharp/DamienG.Library/System/AutoOperators.cs
| { | ||
| if (object.ReferenceEquals(other, null)) { return false; } | ||
| if (object.ReferenceEquals(this, other)) { return true; } | ||
| return |
There was a problem hiding this comment.
Should we check if this and other types are the same? What if someone try to compare EquatableDerivedClass with EquatableDerivedFromDerivedClass? As far as I understood the current implementation will returns true is all local variables would be equal.
There was a problem hiding this comment.
If the two objects are not of the same type then base.Equals will return false.
|
Closing the PR as it is only a POC. See CSHARP-3315 for new examples of Equals methods implemented in this standard way. |
No description provided.